libxl: sane disk backend selection and validation

Introduce a new function libxl__device_disk_set_backend which
does some sanity checks and determines which backend ought to be used.

If the caller specifies LIBXL_DISK_BACKEND_UNKNOWN (which has the
value 0), it tries PHY, TAP and QDISK in that order.  Otherwise it
tries only the specified value.

libxl__device_disk_set_backend (and its helper function
disk_try_backend) inherit the role (and small amounts of the code)
from validate_virtual_disk.  This is called during do_domain_create
and also from libxl_disk_device_add (for the benefit of hotplug
devices).

It also now takes over the role of the scattered fragments of backend
selection found in libxl_device_disk_add,
libxl_device_disk_local_attach and libxl__need_xenpv_qemu.  These
latter functions now simply do the job for the backend they find has
already been specified and checked.

The restrictions on the capabilities of each backend, as expressed in
disk_try_backend (and to an extent in libxl_device_disk_local_attach)
are intended to be identical to the previous arrangements.

In 23618:3173b68c8a94 combined with 23622:160f7f39841b,
23623:c7180c353eb2, "xl" effectively became much more likely to select
TAP as the backend.  With this change to libxl the default backend
selected by the libxl__device_disk_set_backend is intended to once
again to be PHY where possible.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index fef5b6b08593698c89c3b87225fbcb29abb6c3e4..d613ced6952bbc2df88069c00dd210ef02a4e57c 100644 (file)
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -902,49 +902,6 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
 
 /******************************************************************************/
 
-static int validate_virtual_disk(libxl__gc *gc, char *file_name,
-                                 libxl_device_disk *disk)
-{
-    libxl_ctx *ctx = libxl__gc_owner(gc);
-    struct stat stat_buf;
-    char *delimiter;
-
-    if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
-        if (disk->is_cdrom)
-            return 0;
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Empty disk %s is not a CDROM device\n",
-                   disk->vdev);
-        return ERROR_INVAL;
-    }
-
-    if (disk->format == LIBXL_DISK_FORMAT_RAW) {
-        delimiter = strchr(file_name, ':');
-        if (delimiter) {
-            if (!strncmp(file_name, "vhd:", sizeof("vhd:")-1)) {
-                disk->format = LIBXL_DISK_FORMAT_VHD;
-                file_name = ++delimiter;
-            }
-        }
-    }
-
-    if ( stat(file_name, &stat_buf) != 0 ) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s", file_name);
-        return ERROR_INVAL;
-    }
-    if (disk->backend == LIBXL_DISK_BACKEND_PHY) {
-        if ( !(S_ISBLK(stat_buf.st_mode)) ) {
-            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not a block device!\n",
-                file_name);
-            return ERROR_INVAL;
-        }
-    } else if ( S_ISREG(stat_buf.st_mode) && stat_buf.st_size == 0 ) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is 0!\n", file_name);
-        return ERROR_INVAL;
-    }
-
-    return 0;
-}
-
 int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
@@ -955,10 +912,6 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
     libxl__device device;
     int major, minor, rc;
 
-    rc = validate_virtual_disk(&gc, disk->pdev_path, disk); 
-    if (rc)
-        return rc;
-
     front = flexarray_make(16, 1);
     if (!front) {
         rc = ERROR_NOMEM;
@@ -991,34 +944,6 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
     device.domid = domid;
     device.kind = DEVICE_VBD;
 
-
-    /*
-     * Fixing the incoming backend type to try to decide on which
-     * backend to use.  Unfortunately at the moment this code is
-     * utterly broken, but it more or less works.
-     */
-
-    /*
-     * Backend type UNKNOWN should mean "caller does not want to specify",
-     * not "break pointlessely".  (Callers should not be required to
-     * specify the backend if they don't want to.)
-     */
-    if (disk->backend == LIBXL_DISK_BACKEND_UNKNOWN)
-        disk->backend = LIBXL_DISK_BACKEND_TAP;
-
-    /* If blktap is not available then fallback to qdisk */
-    if (disk->backend == LIBXL_DISK_BACKEND_TAP && !libxl__blktap_enabled(&gc))
-        disk->backend = LIBXL_DISK_BACKEND_QDISK;
-
-    /*
-     * blktap cannot handle empty disks (aka cdroms). Fall back to
-     * qdisk because qemu-xen creates the disk based on the xenstore
-     * entries.
-     */
-    if (disk->backend == LIBXL_DISK_BACKEND_TAP &&
-        disk->format == LIBXL_DISK_FORMAT_EMPTY)
-        disk->backend == LIBXL_DISK_BACKEND_QDISK;
-
     switch (disk->backend) {
         case LIBXL_DISK_BACKEND_PHY:
             dev = disk->pdev_path;
@@ -1138,68 +1063,49 @@ char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
     libxl__gc gc = LIBXL_INIT_GC(ctx);
     char *dev = NULL;
     char *ret = NULL;
+    int rc;
 
-    /*
-     * Fixing the incoming backend type to try to decide on which
-     * backend to use.  Unfortunately at the moment this code is
-     * utterly broken, but it more or less works.
-     */
-
-    /*
-     * Backend type UNKNOWN should mean "caller does not want to specify",
-     * not "break pointlessely".  (Callers should not be required to
-     * specify the backend if they don't want to.)
-     */
-    if (disk->backend == LIBXL_DISK_BACKEND_UNKNOWN)
-        disk->backend = LIBXL_DISK_BACKEND_TAP;
+    rc = libxl__device_disk_set_backend(&gc, disk);
+    if (rc) goto out;
 
     switch (disk->backend) {
         case LIBXL_DISK_BACKEND_PHY:
-            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "physical block device must"
-                    " be raw");
-                break;
-            }
-            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching PHY disk %s to domain 0",
-                disk->pdev_path);
+            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk %s",
+                       disk->pdev_path);
             dev = disk->pdev_path;
             break;
         case LIBXL_DISK_BACKEND_TAP:
-            if (disk->format == LIBXL_DISK_FORMAT_VHD ||
-                disk->format == LIBXL_DISK_FORMAT_RAW)
-            {
-                if (libxl__blktap_enabled(&gc))
-                    dev = libxl__blktap_devpath(&gc, disk->pdev_path, disk->format);
-                else {
-                    if (disk->format != LIBXL_DISK_FORMAT_RAW) {
-                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "tapdisk2 is required"
-                            " to open a vhd disk");
-                        break;
-                    } else {
-                        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching tap disk %s to domain 0",
-                            disk->pdev_path);
-                        dev = disk->pdev_path;
-                        break;
-                    }
-                }
+            switch (disk->format) {
+            case LIBXL_DISK_FORMAT_RAW:
+                /* optimise away the early tapdisk attach in this case */
+                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching"
+                           " tap disk %s directly (ie without using blktap)",
+                           disk->pdev_path);
+                dev = disk->pdev_path;
                 break;
-            } else if (disk->format == LIBXL_DISK_FORMAT_QCOW ||
-                       disk->format == LIBXL_DISK_FORMAT_QCOW2) {
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach a qcow or qcow2 disk image");
+            case LIBXL_DISK_FORMAT_VHD:
+                dev = libxl__blktap_devpath(&gc, disk->pdev_path,
+                                            disk->format);
                 break;
-            } else {
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
-                    "type: %d", disk->backend);
+            case LIBXL_DISK_FORMAT_QCOW:
+            case LIBXL_DISK_FORMAT_QCOW2:
+                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach"
+                           " a qcow or qcow2 disk image");
+                break;
+            default:
+                LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                           "unrecognized disk format: %d", disk->format);
                 break;
             }
+            break;
         case LIBXL_DISK_BACKEND_QDISK:
             if (disk->format != LIBXL_DISK_FORMAT_RAW) {
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach a qdisk "
-                    "image if the format is not raw");
+                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
+                           " attach a qdisk image if the format is not raw");
                 break;
             }
-            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching qdisk %s to domain 0\n",
-                disk->pdev_path);
+            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
+                       disk->pdev_path);
             dev = disk->pdev_path;
             break;
         default:
@@ -1208,6 +1114,7 @@ char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
             break;
     }
 
+ out:
     if (dev != NULL)
         ret = strdup(dev);
     libxl__free_all(&gc);

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1b7fd610340dc21ae4471092d5b0b3f5aaa97793..b74b66f6e2f6e393d39a97714c51589550a3f9c2 100644 (file)
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -424,6 +424,12 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
             goto error_out;
     }
 
+
+    for (i = 0; i < d_config->num_disks; i++) {
+        ret = libxl__device_disk_set_backend(gc, &d_config->disks[i]);
+        if (ret) goto error_out;
+    }
+
     if ( restore_fd < 0 ) {
         ret = libxl_run_bootloader(ctx, &d_config->b_info, d_config->num_disks > 0 ? &d_config->disks[0] : NULL, domid);
         if (ret) {

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index e4c98eec77ea683cf2d1db212ba794fa70992cfd..0a00b246716b37d76961dec7711548eb6114d04e 100644 (file)
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -118,6 +118,122 @@ out:
     return rc;
 }
 
+typedef struct {
+    libxl__gc *gc;
+    libxl_device_disk *disk;
+    struct stat stab;
+} disk_try_backend_args;
+
+static int disk_try_backend(disk_try_backend_args *a,
+                            libxl_disk_backend backend) {
+    /* returns 0 (ie, DISK_BACKEND_UNKNOWN) on failure, or
+     * backend on success */
+    libxl_ctx *ctx = libxl__gc_owner(a->gc);
+    switch (backend) {
+
+    case LIBXL_DISK_BACKEND_PHY:
+        if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
+              a->disk->format == LIBXL_DISK_FORMAT_EMPTY)) {
+            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
+                       " unsuitable due to format %s",
+                       a->disk->vdev,
+                       libxl_disk_format_to_string(a->disk->format));
+            return 0;
+        }
+        if (a->disk->format != LIBXL_DISK_FORMAT_EMPTY &&
+            !S_ISBLK(a->stab.st_mode)) {
+            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
+                       " unsuitable as phys path not a block device",
+                       a->disk->vdev);
+            return ERROR_INVAL;
+        }
+
+        return backend;
+
+    case LIBXL_DISK_BACKEND_TAP:
+        if (!libxl__blktap_enabled(a->gc)) {
+            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap"
+                       " unsuitable because blktap not available",
+                       a->disk->vdev);
+            return 0;
+        }
+        if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY ||
+            (S_ISREG(a->stab.st_mode) && !a->stab.st_size)) {
+            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap"
+                       " unsuitable because empty devices not supported",
+                       a->disk->vdev);
+            return 0;
+        }
+        return backend;
+
+    case LIBXL_DISK_BACKEND_QDISK:
+        return backend;
+
+    default:
+        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend "
+                   " %d unknown", a->disk->vdev, backend);
+        return 0;
+        
+    }
+}            
+
+int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk) {
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    libxl_disk_backend ok;
+    disk_try_backend_args a;
+
+    a.gc = gc;
+    a.disk = disk;
+
+    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s spec.backend=%s",
+               disk->vdev,
+               libxl_disk_backend_to_string(disk->backend));
+
+    if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
+        if (!disk->is_cdrom) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s is empty"
+                       " but not cdrom",
+                       disk->vdev);
+            return ERROR_INVAL;
+        }
+        memset(&a.stab, 0, sizeof(a.stab));
+    } else {
+        if (stat(disk->pdev_path, &a.stab)) {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
+                             "failed to stat: %s",
+                             disk->vdev, disk->pdev_path);
+            return ERROR_INVAL;
+        }
+        if (!S_ISBLK(a.stab.st_mode) &
+            !S_ISREG(a.stab.st_mode)) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
+                             "phys path is not a block dev or file: %s",
+                             disk->vdev, disk->pdev_path);
+            return ERROR_INVAL;
+        }
+    }
+
+    if (disk->backend != LIBXL_DISK_BACKEND_UNKNOWN) {
+        ok= disk_try_backend(&a, disk->backend);
+    } else {
+        ok=
+            disk_try_backend(&a, LIBXL_DISK_BACKEND_PHY) ?:
+            disk_try_backend(&a, LIBXL_DISK_BACKEND_TAP) ?:
+            disk_try_backend(&a, LIBXL_DISK_BACKEND_QDISK);
+        if (ok)
+            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, using backend %s",
+                       disk->vdev,
+                       libxl_disk_backend_to_string(ok));
+    }
+    if (!ok) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "no suitable backend for disk %s",
+                   disk->vdev);
+        return ERROR_INVAL;
+    }
+    disk->backend = ok;
+    return 0;
+}
+
 char *libxl__device_disk_string_of_format(libxl_disk_format format)
 {
     switch (format) {

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 5e80bc865e93136748b57f267b72c3f511238384..dc110f2c0a66f86d35e1967da79faf417625cce7 100644 (file)
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1001,25 +1001,10 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
     }
 
     if (nr_disks > 0) {
-        int blktap_enabled = -1;
         for (i = 0; i < nr_disks; i++) {
-            switch (disks[i].backend) {
-            case LIBXL_DISK_BACKEND_TAP:
-                if (blktap_enabled == -1)
-                    blktap_enabled = libxl__blktap_enabled(gc);
-                if (!blktap_enabled) {
-                    ret = 1;
-                    goto out;
-                }
-                break;
-
-            case LIBXL_DISK_BACKEND_QDISK:
+            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK) {
                 ret = 1;
                 goto out;
-
-            case LIBXL_DISK_BACKEND_PHY:
-            case LIBXL_DISK_BACKEND_UNKNOWN:
-                break;
             }
         }
     }

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 76ad1f2acafaa80a4166589302bb087aaee45d9f..579188e29f5c9ad6521bccf3d5ebbd94f5b881ef 100644 (file)
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -205,6 +205,7 @@ _hidden void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid);
 /* from xl_device */
 _hidden char *libxl__device_disk_string_of_backend(libxl_disk_backend backend);
 _hidden char *libxl__device_disk_string_of_format(libxl_disk_format format);
+_hidden int libxl__device_disk_set_backend(libxl__gc*, libxl_device_disk*);
 
 _hidden int libxl__device_physdisk_major_minor(const char *physpath, int *major, int *minor);
 _hidden int libxl__device_disk_dev_number(const char *virtpath,